-
Notifications
You must be signed in to change notification settings - Fork 665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add wrapper for mremap #1306
Add wrapper for mremap #1306
Conversation
It looks like the libc crate doesn't have mremap in the FreeBSD CI environment:
I assume I need to config-guard this wrapper, but I'm not sure how to tell which configs ought to include it. I don't see any such config-guard in the libc crate itself: https://docs.rs/libc/0.2.68/src/libc/unix/linux_like/linux/mod.rs.html#2910 Any ideas? |
Added a guard to make it Linux-only, since the man page notes that it's Linux-specific. |
Hmm, I'm fairly certain the CI failure is spurious. It doesn't look related to this PR, and it previously passed on 7a46b52, which should be exactly the same. I'm happy to rerun if desired; is there a nicer way to force a rerun than pushing an empty commit? |
src/sys/mman.rs
Outdated
old_size: size_t, | ||
new_size: size_t, | ||
flags: MapFlags, | ||
new_address: size_t, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't new_address
be optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; I wasn't sure the best way to handle this since Rust doesn't support variadic functions. The argument is ignored if MAP_FIXED
isn't in flags
; i.e. if MAP_FIXED
isn't set the caller can put anything there, and it'll be ignored.
Maybe it'd be a bit clearer to make it an Option<size_t>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized the flags enum is wrong; will fix that and change new_address to be an Option<size_t>
(but lmk if there's another solution you recommend)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Also added a few tests, though in hindsight maybe these should just be doc tests (and drop the second mremap test)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice tests.
src/sys/mman.rs
Outdated
/// | ||
/// # Safety | ||
/// | ||
/// See the `mremap(2)` man page for detailed requirements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a URL to a man page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Looks good. Now it just needs a squash. |
3e40a96
to
6f84f14
Compare
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
🔒 Permission denied Existing reviewers: click here to make asomers-ax a reviewer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bors r+
Build succeeded: |
This was an oversight from nix-rust#1306. Reported-by: @ocadaruma
This was an oversight from nix-rust#1306. Reported-by: @ocadaruma
This was an oversight from nix-rust#1306. Reported-by: @ocadaruma
This was an oversight from nix-rust#1306. Reported-by: @ocadaruma
Fixes #1295